Fix with_annotated decorator so using ArgumentBlock works with groups#1718
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
- Coverage 99.59% 99.57% -0.02%
==========================================
Files 23 23
Lines 5910 5915 +5
==========================================
+ Hits 5886 5890 +4
- Misses 24 25 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
@KelvinChung2000 I'd greatly appreciate your review as this is my first attempt at modifying the code in @neoniobium Since you reported the issue and understand it, I'd also very much appreciate your review to verify that this fixes the underlying problem. |
There was a problem hiding this comment.
While this is working for normal groups, as they are just normal basiclly an "or" collection of all the entry. But applying the same logic to me does not really works on mutually exclusive group and I would suggest rejecting this use case.
I would prefer a more explict API, something like
@dataclass
class Block(ArgumentBlock):
A: ...
B: ...
class App(Cmd):
@with_annotated(groups=(Group("A", "B"), ...))
def do_with_block(self, block_argument: Block) -> None:
...Which is considered a typo today. But it will be clear in all cases. So the implementation for the above will be more like lifting/changing all the field checks from checking the parameter directly to, when it is an ArgumentBlock, checking whether it is part of the field instead. Since ArgumentBlock is a flat expansion in the parser, all the existing logic should be working already.
|
@KelvinChung2000 If you have a better idea for how to handle this, I'm happy to cancel this PR and let you put up another one. Or if you want, feel free to take over this branch and modify as you see fit. |
3c8a604 to
7cc0986
Compare
|
I checked out the PR and did some brief testing. The issue I raise is fully addressed with it. I happily do a technical review later. However I would prefer if it be decided weather this PR will be closed in favor a a different approach before I do that. Edit: Ok, I see that @KelvinChung2000 commit. I assume this PR to be this to be the one that will address the issues. |
neoniobium
left a comment
neoniobium
left a comment
There was a problem hiding this comment.
I have tested the PR in private project. Works as intended. The implementation look clean.
You might also want to update the example? However, do not have a strong opinion on weather this is too much for an example especially since the current example is also rather long.
|
PR LGTM. I called out one place where I think the file header comment for |
3 participants
Cause
When an
ArgumentBlockparameter is passed to@with_annotated(groups=...), the decorator resolves the argument spec based on the function's parameter names (e.g.block_argument). However, when parsing was eventually performed,block_argumenthad been completely expanded into its respective argument fields (e.g.A,B) which didn't possess theblock_argumentname directly, causing them to fall through as ungrouped arguments rather than being mapped to the user-supplied argument groups.Fix
_ArgparseArgumentto store the name of the block parameter it was expanded from (block_param_name)_expand_dataclass_blockto supplyblock_param_namewhen instantiating expanded argumentsbuild_parser_from_functionto properly fall back to looking up targets via theblock_param_nameif an exact name match isn't found intarget_for_link_mutex_group_membershipto mapblock_param_namesto a list of its respective sub-fields, enablingmutually_exclusive_groupsto also enforce constraints across all expanded block fields correctlytest_group_with_argument_block,test_mutex_group_with_argument_block) to explicitly test this functionality intests/test_annotated.pyCloses #1714